Support batched transactions #223
Conversation
|
Warning Rate Limit Exceeded@sideninja has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 11 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent updates encompass enhancements to the blockchain application, including upgrading Go versions, refining error handling, and improving data handling for batch transactions. Notable changes involve modifications in the ingestion and indexing processes to support multiple transactions per block and updates to testing frameworks for robustness in handling new functionalities. Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
This reverts commit 4b169c2.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| @@ -64,8 +64,7 @@ type ReceiptIndexer interface { | |||
| // GetByBlockHeight returns the receipt for the block height. | |||
| // Expected errors: | |||
| // - errors.NotFound if the receipt is not found | |||
| // TODO right now one transaction per block, but this might change in future so the API needs to be updated. | |||
| GetByBlockHeight(height *big.Int) (*gethTypes.Receipt, error) | |||
| GetByBlockHeight(height *big.Int) ([]*gethTypes.Receipt, error) | |||
There was a problem hiding this comment.
Ensure consistent error handling for the GetByBlockHeight method.
The GetByBlockHeight method in the ReceiptIndexer interface is expected to handle errors such as errors.NotFound when the receipt is not found. However, the method now returns a slice of receipts, which implies multiple receipts could be associated with a single block height. This change in functionality should be accompanied by an update in the documentation and error handling strategy to reflect the possibility of returning multiple receipts and handling their associated errors.
| receipts, err := r.receipts.GetByBlockHeight(heights[i]) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| for _, log := range receipt.Logs { | ||
| if exactMatch(log, r.criteria) { | ||
| logs = append(logs, log) | ||
| for _, receipt := range receipts { | ||
| for _, log := range receipt.Logs { | ||
| if exactMatch(log, r.criteria) { | ||
| logs = append(logs, log) | ||
| } |
There was a problem hiding this comment.
Optimize log filtering with concurrent processing.
The current implementation of log filtering in the RangeFilter class processes receipts sequentially. Given that log filtering can be a CPU-intensive operation, especially with a large number of receipts, consider implementing concurrent processing of receipts to improve performance. This could involve using worker pools or parallel processing techniques to handle multiple receipts simultaneously.
storage/pebble/receipts.go
Outdated
| // Store receipt in the index. | ||
| // | ||
| // Storing receipt will create multiple indexes, each receipt has a transaction ID, | ||
| // and a block height. We create following mappings: | ||
| // - receipt transaction ID => block height bytes | ||
| // - receipt block height => list of encoded receipts (1+ per block) | ||
| // - receipt block height => list of bloom filters (1+ per block) | ||
| func (r *Receipts) Store(receipt *gethTypes.Receipt) error { | ||
| r.mux.Lock() | ||
| defer r.mux.Unlock() | ||
|
|
||
| // convert to storage receipt to preserve all values | ||
| rr := (*models.StorageReceipt)(receipt) | ||
| val, err := rlp.EncodeToBytes(rr) | ||
| if err != nil { | ||
| return err | ||
| // we must first retrieve any already saved receipts at the provided height, | ||
| // and if found we must add to the list, because this method can be called multiple | ||
| // times when indexing a single EVM height, which can contain multiple receipts | ||
| receipts, err := r.getByBlockHeight(receipt.BlockNumber.Bytes()) | ||
| if err != nil && !errors.Is(err, errs.ErrNotFound) { // anything but not found is failure | ||
| return fmt.Errorf("failed to store receipt to height, retrieve errror: %w", err) | ||
| } | ||
|
|
||
| // add new receipt to the list of all receipts, if the receipts do not yet exist at the | ||
| // provided height, the above get will return ErrNotFound (which we ignore) and the bellow | ||
| // line will init an empty receipts slice with only the provided receipt | ||
| receipts = append(receipts, receipt) | ||
|
|
||
| batch := r.store.newBatch() | ||
| defer batch.Close() | ||
|
|
||
| // convert to storage receipt to preserve all values | ||
| storeReceipts := make([]*models.StorageReceipt, len(receipts)) | ||
| for i, rr := range receipts { | ||
| storeReceipts[i] = (*models.StorageReceipt)(rr) | ||
| } | ||
|
|
||
| val, err := rlp.EncodeToBytes(storeReceipts) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| height := receipt.BlockNumber.Bytes() | ||
|
|
||
| if err := r.store.set(receiptTxIDToHeightKey, receipt.TxHash.Bytes(), height, batch); err != nil { | ||
| return fmt.Errorf("failed to store receipt tx height: %w", err) | ||
| } | ||
|
|
||
| // todo if there are more transactions per block we need to update this | ||
| if err := r.store.set(receiptHeightKey, height, val, batch); err != nil { | ||
| return fmt.Errorf("failed to store receipt height: %w", err) | ||
| } | ||
|
|
||
| // todo support multiple heights |
There was a problem hiding this comment.
Refactor the Store method to improve error handling and efficiency.
The Store method in the Receipts class performs multiple operations including retrieving existing receipts, appending new receipts, and committing them to the database. The method currently handles errors at multiple points, which can make the code hard to read and maintain. Consider refactoring this method to separate concerns more clearly, perhaps by breaking it down into smaller helper methods that each handle a specific part of the process. This could improve both readability and error handling.
|
@sideninja I think the committed changes to |
There was a problem hiding this comment.
Actionable comments posted: 2
Out of diff range and nitpick comments (2)
storage/pebble/receipts.go (2)
33-39: Clarify the documentation for theStoremethod.The comment block for the
Storemethod could be more descriptive about how the multiple indexes are created and used. Consider expanding this to include examples or more detailed explanations.
Line range hint
178-201: Refactor error handling for range checks.- if start.Cmp(end) > 0 { - return nil, nil, fmt.Errorf("start is bigger than end: %w", errs.ErrInvalidRange) - } - if start.Uint64() > last { - return nil, nil, fmt.Errorf( - "start value %d is not within the indexed range of [0 - %d]: %w", - start, - last, - errs.ErrInvalidRange, - ) - } - if end.Uint64() > last { - return nil, nil, fmt.Errorf( - "end value %d is not within the indexed range of [0 - %d]: %w", - end, - last, - errs.ErrInvalidRange, - ) - } + if start.Cmp(end) > 0 || start.Uint64() > last || end.Uint64() > last { + return nil, nil, fmt.Errorf("invalid range: start %d or end %d is out of bounds [0 - %d]", start, end, last) + }Consolidate the range checks into a single conditional statement to simplify the code and reduce redundancy. This change makes the function easier to read and maintain.
| blockHeight := receipt.BlockNumber.Bytes() | ||
| receipts, err := r.getByBlockHeight(blockHeight) | ||
| if err != nil && !errors.Is(err, errs.ErrNotFound) { // anything but not found is failure | ||
| return fmt.Errorf("failed to store receipt to height, retrieve exisint receipt errror: %w", err) |
There was a problem hiding this comment.
Improve error message clarity.
- return fmt.Errorf("failed to store receipt to height, retrieve exisint receipt errror: %w", err)
+ return fmt.Errorf("failed to store receipt to height, retrieve existing receipt error: %w", err)The error message contains a typo ("exisint" should be "existing") and another typo in "errror". Correcting these will improve the readability and professionalism of the code.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| return fmt.Errorf("failed to store receipt to height, retrieve exisint receipt errror: %w", err) | |
| return fmt.Errorf("failed to store receipt to height, retrieve existing receipt error: %w", err) |
storage/pebble/receipts.go
Outdated
| bloomVal, err := rlp.EncodeToBytes(blooms) | ||
| if err := r.store.set(bloomHeightKey, height, bloomVal, batch); err != nil { |
There was a problem hiding this comment.
Address ineffectual assignment to err.
- bloomVal, err := rlp.EncodeToBytes(blooms)
- if err := r.store.set(bloomHeightKey, height, bloomVal, batch); err != nil {
+ bloomVal, encodeErr := rlp.EncodeToBytes(blooms)
+ if err := r.store.set(bloomHeightKey, height, bloomVal, batch); encodeErr != nil {
return fmt.Errorf("failed to store bloom height: %w", encodeErr)
}The variable err is reassigned before its value is checked, which makes the assignment ineffectual. Use a different variable for the error from the rlp.EncodeToBytes function to ensure that it is effectively checked.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| bloomVal, err := rlp.EncodeToBytes(blooms) | |
| if err := r.store.set(bloomHeightKey, height, bloomVal, batch); err != nil { | |
| bloomVal, encodeErr := rlp.EncodeToBytes(blooms) | |
| if err := r.store.set(bloomHeightKey, height, bloomVal, batch); encodeErr != nil { |
Closes: #224
Description
Add support for multiple transactions in a single block, which is now possible due to
EVM.batchRun(). This PR adds tests blocks with multiple transactions, which was copied from PR made by @m-Peter #222And adds logic to ingest such blocks.
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests